Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Base Docker image on debian:buster-slim #234

Closed
wants to merge 1 commit into from
Closed

Base Docker image on debian:buster-slim #234

wants to merge 1 commit into from

Conversation

lukechilds
Copy link
Contributor

Resolves #226.

The current Docker image has a strange bug when being built on 32 bit ARM devices causing RocksDB to be built incorrectly and electrs to exit as soon as it's started.

Updating to Debian Buster seems to fix the bug somewhere in the C++ toolchain. There is no official Buster Docker image for Rust 1.34.0 so instead I've used Debian Buster as the base image and manually installed Rust 1.34.0.

I've also added RUST_VERSION as a build arg which is passed through to rustup.

This means the image easily be changed to support any Rust version supported by rustup at build time by passing in a build arg or by changing the default in the Dockerfile.

e.g:

# Builds with Rust 1.34.0 by default
$ docker build -t electrs .

# Manually specify a Rust version
$ docker build -t electrs --build-arg RUST_VERSION=1.40.0 .

# Use the latest stable Rust version
$ docker build -t electrs --build-arg RUST_VERSION=stable .

@lukechilds lukechilds mentioned this pull request Apr 13, 2020
Closed
Kixunil
Kixunil reviewed Apr 13, 2020
View reviewed changes
Dockerfile
-y \
--verbose \
--profile minimal \
--default-toolchain $RUST_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to install it using apt-get to get signature validation for free. If you prefer rustup, then I'd suggest doing what the official Docker image does and check the hashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point regarding apt over rustup.

However regarding rustup, it doesn't seem possible to get a secure Rust environment via rustup which was why I intentionally didn't bother with hashes.

For example the official Docker image only checks the hash of rustup-init.sh. This is pointless because:

  1. rustup-init.sh then downloads rustup but doesn't check hashes/sigs.
  2. rustup doesn't check signatures when installing Rust versions.

Se verifying rustup-init.sh doesn't guarantee anything about rustup, and even if you get an honest rustup, that doesn't even guarantee you get an honest rustc/cargo!

But you're right about preferring apt, that would definitely be better.

Dockerfile
--profile minimal \
--default-toolchain $RUST_VERSION

RUN chmod -R a+w $RUSTUP_HOME $CARGO_HOME
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks strange, is it a standard thing to do in docker?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's not a particularly standard thing to do in Docker, normally you would compile a binary then copy it to usr/local/bin or similar.

Not very familiar with Cargo but it seems to want it's own build dir with other data that you then add to your path.

Without changing the permissions the user account doesn't have permission to write to $CARGO_HOME during installation.

The official Rust images do it like this too: https://github.com/rust-lang/docker-rust/blob/8bab191937fcf23569d3a3c31103c1c6f7f2947e/1.42.0/buster/slim/Dockerfile#L30

I guess we could just build as root in /root/.cargo/bin and then copy the electrs binary to /usr/local/bin which should allow user to execute it.

@lukechilds
Copy link
Contributor Author

@Kixunil Good points in your feedback. I just went with rustup due to the fact that it allows us to change the Rust version easily and that's also what the official Docker Rust images are using.

But I'm also not a fan of no signature verification and clunky $CARGO_HOME workarounds.

Installing cargo from apt and moving binaries into /usr/local/bin and not having to worry about $CARGO_HOME permissions seems like an improvement over the official Image.

I'll wait to hear back from @romanz first re #226 (comment). Maybe a super secure Docker env is outside the scope of this project and just a simple functional Dockerfile using the official Rust images is adequate which we now have in 7d23da5.

@Kixunil
Copy link
Contributor

Kixunil commented Apr 13, 2020

Right. If secure docker is out of scope, this should be documented.

@romanz
Copy link
Owner

romanz commented Apr 14, 2020

Thanks!
Merged using #235 instead.

@romanz romanz closed this Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

electrs exits with code 135
3 participants
@lukechilds @Kixunil @romanz